CSHARP-5633: BsonSerializer.IsTypeDiscriminated is not thread-safe #1722
Conversation
| return true; | ||
| } | ||
|
|
||
| __configLock.EnterReadLock(); |
There was a problem hiding this comment.
Could you please double-check if introducing this synchronization affects performance. If so, we could replace HashSet<Type> with ConcurrentDictionary<Type, bool>.
There was a problem hiding this comment.
I changed it to use a ConcurrentDictionary, thanks for taking time to do the benchmarks :)
| @@ -321,8 +321,7 @@ internal static bool IsDiscriminatorConventionRegisteredAtThisLevel(Type type) | |||
| /// <returns>True if the type is discriminated.</returns> | |||
| public static bool IsTypeDiscriminated(Type type) | |||
| { | |||
| var typeInfo = type.GetTypeInfo(); | |||
| return typeInfo.IsInterface || __discriminatedTypes.Contains(type); | |||
| return type.GetTypeInfo().IsInterface || __discriminatedTypes.ContainsKey(type); | |||
There was a problem hiding this comment.
GetTypeInfo is left over from when we used to target a very old .NET Core framework.
I think this line can be just:
return type.IsInterface || __discriminatedTypes.ContainsKey(type);
| @@ -587,7 +586,7 @@ public static void RegisterDiscriminator(Type type, BsonValue discriminator) | |||
| // mark all base types as discriminated (so we know that it's worth reading a discriminator) | |||
| for (var baseType = typeInfo.BaseType; baseType != null; baseType = baseType.GetTypeInfo().BaseType) | |||
| { | |||
| __discriminatedTypes.Add(baseType); | |||
| __discriminatedTypes.TryAdd(baseType, true); | |||
There was a problem hiding this comment.
Is it OK that we are ignoring the return value of TryAdd?
I understand that we expect it to always return true given the check on line 582.
Should we document that we are ignoring the return value on purpose by coding like this:
_ = __discriminatedTypes.TryAdd(baseType, true);
There was a problem hiding this comment.
I think it's safe to ignore the return value of TryAdd in the same way we ignored Add before (the return value has the same semantic).
Maybe adding a comment would make it clearer, I'm not a super fan of adding the discard operator.
There was a problem hiding this comment.
Interesting... I am a fan of the discard variable. I always prefer code to comments to document something.
But it's fine either way.
| @@ -587,7 +586,7 @@ public static void RegisterDiscriminator(Type type, BsonValue discriminator) | |||
| // mark all base types as discriminated (so we know that it's worth reading a discriminator) | |||
| for (var baseType = typeInfo.BaseType; baseType != null; baseType = baseType.GetTypeInfo().BaseType) | |||
| { | |||
| __discriminatedTypes.Add(baseType); | |||
| __discriminatedTypes.TryAdd(baseType, true); | |||
There was a problem hiding this comment.
Interesting... I am a fan of the discard variable. I always prefer code to comments to document something.
But it's fine either way.
CSHARP-5633